Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary panic #5427

Merged
merged 1 commit into from
Aug 23, 2024
Merged

Remove unnecessary panic #5427

merged 1 commit into from
Aug 23, 2024

Conversation

eskimor
Copy link
Member

@eskimor eskimor commented Aug 20, 2024

There is no reason to panic here.

`validity_vote` function is also called from `import_statement` not only
from `import_candidate`. The proof still holds if we assume seconded
statements always come before valid statements, which should be the case
but is not actually enforced by this module at all. On top of this the
proof is not local.
@eskimor eskimor added the T0-node This PR/Issue is related to the topic “node”. label Aug 20, 2024
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@alindima
Copy link
Contributor

validity_vote function is also called from import_statement not only from import_candidate

it cannot panic here if called from import_statement, as the validity_vote call is hardcoded to use ValidityVote::Valid.
I think the root cause would be: #5421 (comment).

But indeed we could at least not panic here

@eskimor eskimor added the R0-silent Changes should not be mentioned in any release notes label Aug 21, 2024
@sandreim
Copy link
Contributor

@eskimor a PRDoc would be nice as we want to backport this to stable.

@alindima
Copy link
Contributor

And please update the PR description as that will be used for the commit message. The current description does not describe the real reason why we see this failing

@eskimor eskimor mentioned this pull request Aug 23, 2024
5 tasks
@eskimor eskimor added this pull request to the merge queue Aug 23, 2024
@eskimor eskimor changed the title Remove panic, as proof is invalid. Remove unnecessary panic Aug 23, 2024
Merged via the queue into master with commit d26d7f1 Aug 23, 2024
255 of 263 checks passed
@eskimor eskimor deleted the rk-remove-panic branch August 23, 2024 14:32
@eskimor eskimor restored the rk-remove-panic branch August 24, 2024 07:14
@eskimor eskimor deleted the rk-remove-panic branch August 24, 2024 07:14
eskimor added a commit that referenced this pull request Aug 24, 2024
`validity_vote` function is also called from `import_statement` not only
from `import_candidate`. The proof still holds if we assume seconded
statements always come before valid statements, which should be the case
but is not actually enforced by this module at all. On top of this the
proof is not local.

Co-authored-by: eskimor <[email protected]>
ggwpez pushed a commit that referenced this pull request Aug 27, 2024
`validity_vote` function is also called from `import_statement` not only
from `import_candidate`. The proof still holds if we assume seconded
statements always come before valid statements, which should be the case
but is not actually enforced by this module at all. On top of this the
proof is not local.

Co-authored-by: eskimor <[email protected]>
ordian added a commit that referenced this pull request Aug 27, 2024
* master: (36 commits)
  Bump the ci_dependencies group across 1 directory with 2 updates (#5401)
  Remove deprecated calls in cumulus-parachain-system (#5439)
  Make the PR template a default for new PRs (#5462)
  Only log the propagating transactions when they are not empty (#5424)
  [CI] Fix SemVer check base commit (#5361)
  Sync status refactoring (#5450)
  Add build options to the srtool build step (#4956)
  `MaybeConsideration` extension trait for `Consideration` (#5384)
  Skip slot before creating inherent data providers during major sync (#5344)
  Add symlinks for code of conduct and contribution guidelines (#5447)
  pallet-collator-selection: correctly register weight in `new_session` (#5430)
  Derive `Clone` on `EncodableOpaqueLeaf` (#5442)
  Moving `Find FAIL-CI` check to GHA (#5377)
  Remove panic, as proof is invalid. (#5427)
  Reactive syncing metrics (#5410)
  [bridges] Prune messages from confirmation tx body, not from the on_idle (#5006)
  Change the chain to Rococo in the parachain template Zombienet config (#5279)
  Improve the appearance of crates on `crates.io` (#5243)
  Add initial version of `pallet_revive` (#5293)
  Update OpenZeppelin template documentation (#5398)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants